Skip to content

deriving should give expn_info #13150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

deriving should give expn_info #13150

wants to merge 1 commit into from

Conversation

nrc
Copy link
Member

@nrc nrc commented Mar 26, 2014

Ensure that all generated code from deriving attributes get an expn_info in their span.

Ensure that all generated code from deriving attributes get an expn_info in their span.
@nrc
Copy link
Member Author

nrc commented Mar 26, 2014

TBH, I have no idea if the call site, name, and span are right. It would be nice to have some more documentation around these things.

@emberian
Copy link
Member

cc @huonw

@huonw
Copy link
Member

huonw commented Mar 26, 2014

Looks fine to me, but I'm not 100% sure about what the fields of expn_info are either; I'll investigate that, report back/submit docs and then (presumably) r+ in a few hours.

(Someone else is free to do these steps too.)

@huonw
Copy link
Member

huonw commented Mar 26, 2014

#13152

While doing that I poured over the expansion source, and I'm thoroughly confused as to why this specific change (and similar changes I've done in the past) have any effect, specifically: there's this new_span function that, afaict, is called on every single span (except for the single one I caught in that PR) in every single expansion, rewriting the expn_info based on the current macro backtrace.

@nick29581 Can you double check that this change has the effect you need for DXR? (Also, derived implementations get the attribute #[automatically_derived] now (although I'm inclined to change the exact wording of that attribute, since the #[deriving] attribute is certainly not automatically applied).)

@nrc
Copy link
Member Author

nrc commented Mar 26, 2014

@huonw I don't think new_span is called on every single span, see for example expand_deriving_clone in clone.rs where we just set the span to the span passed in from expand_meta_deriving. That is why I added the assert there. Possibly that is a bug, I don't understand this code well enough to tell. Also new_span can set expn_info to None if cx.backtrace() is None, which is possible (I fixed exactly such a bug a while ago). So I think this change is required as DXR exists at the moment.

I guess I could check automatically_derived (I'm glad it is there), but I do think in general it is nice if all generated code has some expn_info, not just for DXR.

@nrc
Copy link
Member Author

nrc commented Mar 26, 2014

And I don't know what that travis fail is about, passes that test locally.

@huonw
Copy link
Member

huonw commented Mar 27, 2014

I'd guess the travis failure was just a spurious one (it's passing now).

I don't think new_span is called on every single span

I think it is, since every item has expand_item called on it, including the items that macros create (and expand_item calls noop_fold_item which then runs the rest of the folding, including new_spans on everything). However, this happens outside #[deriving], i.e. the spans are set after deriving returns so the assert inside expand_deriving_clone isn't seeing the rewritten span.

I could easily be reading the code incorrectly, though.

@flaper87
Copy link
Contributor

(btw, if the travis failure seems spurious, it's possible to restart the job from travis' UI, I did that yday for this PR and then forgot to comment here)

@nrc
Copy link
Member Author

nrc commented Mar 31, 2014

@huonw looks like you're right, as far as I can tell - at least without this patch, DXR still produces the same output. This was definitely not the case at some point, but DXR tests are much better now, so hopefully this won't regress again.

@nrc nrc closed this Mar 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants